Skip to content

Fix/improve error handling #134

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Apr 25, 2018
Merged

Fix/improve error handling #134

merged 5 commits into from
Apr 25, 2018

Conversation

@DickSmith
Copy link

@DickSmith DickSmith commented Apr 17, 2018 •
edited
Loading

Included latest typings from net.gotev.uploadservice which shows a change in the signature of onError from:
onError(context: any, uploadInfo: UploadInfo, error) {}
to:
onError(context: Context, uploadInfo: UploadInfo, serverResponse: ServerResponse, error: java.lang.Exception) {}
Also, the error was also not being passed to the new zonedOnError function.

Also added the responseCode in the ErrorEventData in a cross-platform matter to allow apps to respond accordingly.

PR Checklist

What is the current behavior?

ServerResponse is being passed as the error rather than the java.lang.Exception in the current method, but then isn't actually being passed to zonedOnError, either.
The response code is not available.

What is the new behavior?

The Exception is now being passed correctly as the error.
The responseCode is now available for both iOS and Android, or -1 if the response is invalid.

BREAKING CHANGES:
None, since currently the error isn't being passed up at all on Android, and the responseCode is a new property for both.

Fix/improve error handling
6b6fbaa 
Included latest typings from `net.gotev.uploadservice` which shows a change in the signature of `onError` from:
`onError(context: any, uploadInfo: UploadInfo, error) {}`
to:
`onError(context: Context, uploadInfo: UploadInfo, serverResponse: ServerResponse, error: java.lang.Exception) {}`
Also, the `error` was also not being passed to the new `zonedOnError` function.

Also added the responseCode in the `ErrorEventData` in a cross-platform matter to allow apps to respond accordingly.
@ghost ghost added the new PR label Apr 17, 2018
VladimirAmiorkov
VladimirAmiorkov suggested changes Apr 20, 2018
View reviewed changes
Copy link
Contributor

@VladimirAmiorkov VladimirAmiorkov left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DickSmith I think that it would be good to also add the responseCode to the complete event that is raised by onProgressReceiverCompleted and URLSessionTaskDidCompleteWithError.

Other than that I tested the PR and it looks good. Could you make the above changes after which I will proceed with merging and publishing the package.

Per request, adding responseCode to ResultEventData
b4d4435 
Add type-checking to Error, Result, and Progress payloads.
Convert tabs to spaces in `background-http.ios.ts` to be consistent with rest of repo
@DickSmith
Copy link
Author

DickSmith commented Apr 23, 2018

@VladimirAmiorkov
I added the responseCode to Result per your request.
I also switched background-http.ios.ts to use spaces rather than tabs to make it consistent with the other files.

@DickSmith
Copy link
Author

DickSmith commented Apr 23, 2018

Ah, I just reread your comment.

What I was thinking of doing, Android is already returning the response object there, but iOS is not, do you still want the response returned there? or just the responseCode?

Per Request, creating interface for CompleteEventData
b2f214a 
Adding `responseCode` to CompleteEventData
Breaking for Android: Android originally returned the response here (undocumented behaviour), while iOS did not, so now only the code will be returned
@DickSmith
Copy link
Author

DickSmith commented Apr 23, 2018

OK, so I added the responseCode to complete as well.
I removed the response that only Android was putting there, I could add the "response" to the iOS data as well, but these will be platform specific and the responseCode should suffice better, so potentially a breaking change on Android, but sending the full response object in Android was probably too heavy and unused anyway.

Let me know your thoughts.

@VladimirAmiorkov
Copy link
Contributor

VladimirAmiorkov commented Apr 24, 2018

Hi @DickSmith ,

I see what you mean about the response inside the complete event on Android and I agree that it could be redundant if you only want to retrieve the response code from it. But that ServerResponse contains additional functionality like getHeaders() etc. that brings value to the event arguments. Just for reference the same can be achieve on iOS using the object like so args.object.ios.response.allHeaderFields so removing the response entirely from the event in Android should not be done.

I would suggest to either expose the native response on iOS also which is currently a missing functionality (two birds with one stone) or simply return the response inside the complete event on Android and simply add the new responseCode so I can proceed with merging this PR. I understand that providing platform specific objects in event callbacks is not great but sometimes it is better to provide the entire native object than adding multiple properties one by one to the arguments with each new version of the plugin. We will be happy if you could help us with achieving full platform unity in this API but I suggest you to do it in a separate PR to not make this one too big and keep it relevant to the responseCode exposure.

We appreciate your work on this.

@DickSmith
Copy link
Author

DickSmith commented Apr 24, 2018

@VladimirAmiorkov
Ah, for sure, I could've go either way on it.
If we want the response to actually be exposed then we should add it to the CompleteEventData interface so that it is clearer that it is intended. That was the reason I defaulted to removing it in this case, since it wasn't declared in any of the typings and wasn't on the iOS, so I was unsure if it was intended or not, or just an artifact of previous versions of the plugin or for debug during development that got left behind.

I'll add a JSDoc to the response object and detail that it is currently "android-only" for now.

Would be nice to unify those for sure, but yeah, I don't currently have the time to plow deeper on this one myself, and the responseCodes satisfy all my current needs.

@DickSmith
Copy link
Author

DickSmith commented Apr 24, 2018

@VladimirAmiorkov
Done, let me know if that works.

@VladimirAmiorkov VladimirAmiorkov merged commit e69ba5f into NativeScript:master Apr 25, 2018
@ghost ghost removed the new PR label Apr 25, 2018
@VladimirAmiorkov
Copy link
Contributor

VladimirAmiorkov commented Apr 25, 2018 •
edited
Loading

@DickSmith This is available with version 3.2.5

@Burgov Burgov mentioned this pull request May 15, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@VladimirAmiorkov VladimirAmiorkov VladimirAmiorkov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@DickSmith @VladimirAmiorkov